I think it's incohesive to define such a specific message type here.

The message bus common package is meant to house logic and types that are common between the message bus service and the message bus client. In general I think it makes sense to define a generic message envelope in here, but I think defining the structure of messages that are specific to some other package (compile-common in this case) is probably a code smell.

The easy test for why it's problematic is that you need to take a dependency on compile-common here. Perhaps this could be avoided by defining a more generic and extensible message envelope type here that you could use to embed this same information? For example, for messages to do with RPC calls we just have the DashboardProviderMessage type. We don't have one message type per type of RPC call, however.

Perhaps there's a more generic CLIEventMessageType structure that would work better here?

If you aren't familiar with how other message busses work, it might be worth having a look at the message envelope types used by MQTT and AMQP.

In general I think it makes sense to define a generic message envelope in here, but I think defining the structure of messages that are specific to some other package (compile-common in this case) is probably a code smell.

Put differently - there's no reason why any of the core message bus service logic must know anything at all about WorkflowCompileResult in order do its job effectively. It should be able to treat that as opaque data that it's just shuffling around.

Thanks Ben. Initially I was uncertain about having an envelope message type that represents any CLI event data, mostly because the payload must necessarily be typed any (I've typed payload data unknown by default). But I figured that I can just type it WorkflowCompileResult in the frontend, which admittedly isn't as nice as the given type guarantees ensured by isWorkflowCompileResultMessage, but this is a worthwhile tradeoff compared to taking compile-common as a dependency and making more specific message types in the future as dashboard grows, which does smell.

I've made updates to use CliEventMessage and got rid of WorkflowCompileResultMessage.

I think this is fine as-is for now, but you might want to consider making the type of data a generic just to force the consumer/producer to type things properly at the ingress/egress points.

Otherwise in the future it'd be great if the payload portion of this type into a new @truffle/event-messages package, or some other existing place like @truffle/events.

If you do that, then you can make use of discriminated unions to avoid the any, and more importantly to make it really easy to ensure that you've handled all potential types of payload in the inevitable switch/case statement that you'll have to write in the frontend where incoming events are handled.

Personally in other projects I've gone with the (admittedly annoying) approach of creating new modules to contain pure types like these, as otherwise you have to take a dependency on a much "fatter" package (like @truffle/events) from frontend code.

All of that said, if we go that route, I'd much prefer to see that done in a future PR, as I think this is fine for now and it'll unblock my work on TypeScript support.

As an aside, this is more reason to switch over to aedes/MQTT, as if we were using that we'd have been confronted with the need to split these message types into packages that are specific to their function.

Thanks @benjamincburns. I'll definitely dig into other message brokers (especially MQTT) in the near future as dashboard's messaging requirements grow. Though I do wonder if adding another dashboard package is better than squishing the types into @truffle/dashboard-message-bus-common

The message bus common package is meant to house logic and types that are common between the message bus service and the message bus client

Something like @truffle/event-messages would serve the purpose of providing types to all things message bus, which falls into the domain of the common package?

Ah and about using generics here: I considered it but since the entire message lifecycle for CliEventMessage isn't useful at the moment, (this may very well change; right now it's just abandoned after publish, so only message.payload.data is useful), I opted to simply pass the payload data to a function (handleWorkflowCompileResult) whose params are typed.

Something like @truffle/event-messages would serve the purpose of providing types to all things message bus, which falls into the domain of the common package?

My thinking in the case of a hyptothetical @truffle/event-messages was that the only consumers of that package would be event message producers or consumers. @truffle/dashboard-message-bus-common is really only meant to contain code that's common to both the message bus client and the message bus service itself, and my thought was that it should only contain logic and/or types that is strictly required for the proper functioning of both. In my ideal world the only direct consumers of @truffle/dashboard-message-bus-common would be @truffle/dashboard-message-bus and @truffle/dashboard-message-bus-client.

Honestly it's probably a mistake on my part to have created the common package without moving the message payload types elsewhere.

Ah and about using generics here: I considered it but since the entire message lifecycle for CliEventMessage isn't useful at the moment, (this may very well change; right now it's just abandoned after publish, so only message.payload.data is useful), I opted to simply pass the payload data to a function (handleWorkflowCompileResult) whose params are typed.

You may want to consider changing the type of data from any to unknown, as that will force an explicit cast. I don't feel strongly about that preference, however. As a pattern it makes the code that consumes the unknown-typed field more explicit and readable, but in this specific case I don't think that it's likely to add a ton of extra value over any.

You may want to consider changing the type of data from any to unknown

Done!

How do we know whether we've chosen the correct version of the workflow compile result for the transaction in question? I see that everything is keyed on this sha1 hash of the entire WorkflowCompileResult, but that won't allow for matching to a specific version of a given contract, will it?

A better approach might be to store off this hash, but also an index of compliations that's keyed by the hash of the deployed contract bytecode. That would let you verify if you're decoding against the correct/deployed version by hashing the result of eth_getCode and checking for it in the database.

This would also allow for less arbitrary cache invalidation strategies, as you could treat the database as an LRU cache and only keep around the most recently used N versions of each contract, where N is ideally some user-configurable value.

Yea so the db essentially maps compilation hash to compilation + timestamp.

I see two things here:

What if workflow-compile changes?

At the moment it's only dependent on the shape of Compilation, so it that changes, it's probably time to migrate db.

The current schema means we can't match, hence decode specific version of a given contract?

So the reason for storing compilations is that one compilation is the unit; you cannot break it into e.g. sources, contracts, etc. and reconstruct it at a later time. That being said, all compilations are fed to one decoder, which means if you write contract A, compile, modify A to be A', compile, the decoder will use compilation for A' to decode.

One caveat as noted by this comment is that in some cases the bytecodes are the same despite having different contracts (like a change in param name), in which case with this week's release the decoder will prefer the "latest" compilation.

Could this be a devDependency?

Oh I guess you might be using it for the shims.

for the shims

Nah just using the types for now.

ah so then maybe it can be a devDep!

@gnidan I thought your opinion on these types (which are exposed through TS magic) was that they should really be regular dependencies

Wait, I assumed that dashboard didn't expose these... are they exposed?

Nope it's all lost at compile time. It's used here and here.

Updated to be devDep!

I thought the reason to use deps was to make sure types don't break when the project is added as a dependency of another project via npm/yarn.

I thought the reason to use deps was to make sure types don't break when the project is added as a dependency of another project via npm/yarn.

Not sure what you mean here, but we only need a regular dep if the package runs the dependency's code (trivial case) or if the package exports something that references one of the dependency's types, like as argument or return value for an exported function (case we're discussing)

What's the value of a generic cli-event, vs. making a dedicated workflow-compile-result message type? Do we really want to shoehorn a bunch of different events like this? I know Ben was talking about making things generic to avoid the use of any, but we could also... just make message types for message types we expect.

I'm torn, Ben's most convincing comment imo is this one. Maybe we can reconsider this one when dashboard starts receiving CLI events of more types?

As an aside, I just thought about the plugin system and it will probably require a generic message type similar to this one.

FWIW, my biggest (albeit not my only) concern about the specific message type was that it was causing us to take a dependency on workflow-compile from dashboard-message-bus-common.

My thought there is that over time as the scope of the dashboard grows, it's likely going to require data from a bunch of different packages. My fear is that if we establish a pattern of coupling generic/core/common packages to specific/functional packages we're likely going to run into a bunch of obnoxious problems (e.g. circular dependencies) down the line. Admittedly in this case the logic wasn't tightly coupled, but in my view that's more reason to avoid taking the direct dependency.

As a secondary concern, I think it's also usually a bad sign for the maintainability of an architecture when "transport" systems need to be made aware of the detailed structure of the data that it's passing. Ideally we'd have a single envelope type for all messages, there would be some topic key in that type, and the bus would just route messages to relevant subscribers based on that topic key. The payload field of that message envelope would also ideally be typed unknown so that we'd get compiler errors if anything w/i the message bus logic tried to interact with it directly.

Apologies if I missed it, but is this now using the new interface to add compilations to the decoder?

(If it's not already using this interface, it can probably be a separate task if you want, but should at least open an issue to do that switch.)

Yea I'll leave that as a follow-up, we still re-init the decoder on new compilations.
I'll open an issue once this is merged, thanks for the reminder!